-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes secret marshaling for kube play. Merge stringData with data for secrets. #16631
Fixes secret marshaling for kube play. Merge stringData with data for secrets. #16631
Conversation
d60bec5
to
c6dc1fe
Compare
ca0f876
to
8530bc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing!
Changes LGTM
@cdoern PTAL
@odra PTAL |
@ygalblum PTAL |
@ashley-cui @umohnani8 PTAL |
This change mixes I can see that you changed the test so that it looks for the decoded value, but I don't understand what made it work now |
I tried to run the pr locally but I got the following error:
secret.yaml:
pod.yaml:
|
The base64 decoding happens on unmarshaling. The problem was that the entire secret object was serialized and stored in the (podman) secret while only the data of it was expected to be loaded when using it to fill env vars or volumes - hence the error. Also K8s states that content of secrets in volumes and environment variables should be decoded and also that stringData will be put on top of data (which was no the case previously). |
@odra podman on fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱ podman on fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱ podman on fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱ podman on fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱ |
Ran it with --replace and no issue. |
Sorry for nagging, I'm just trying to understand why before the change the values in the container were encoded and now they are not. Your change I understand. Instead of storing the entire K8S secret object as Podman secret, you store only its data by combining Furthermore, I now understand that the decoding is handled by the But, what I fail to understand is why before this change the values mounted into the container were encoded and now they are decoded. Now, don't get me wrong, I agree they should be decoded to align with K8S. I just don't understand how it happened. Thanks in advance |
I haven't really checked it but might come from the fact that play used "github.com/ghodss/yaml" for marshaling while volume used "gopkg.in/yaml.v3" for unmarshaling. |
OK, thanks |
LGTM, but I guess @odra should first see what's not working for him |
Will this break existing kube secrets? Since this changes the secret storage schema? I don't remember if kube secrets persist across plays though, so this might not be a problem.. |
I think @ashley-cui is right. K8S secrets are stored into Podman secrets and hence persist. Since this change breaks the schema, podman will fail to load secrets that were created with an older version |
Maybe that's the issue @odra is having. |
Yeah, that's the issue I had, storing secrets with "play kube" works wihile storing a raw kubernetes secret with "podman secret create.." breaks it (altough that was working before this change) but creating it wth "play kube" works. I thought that creating a k8s secret using "play kube" would store the raw yaml/json secret as podman secret but that doesn't seem to be the case right? Using "play kube" to store kubernetes secrets sound a bit weird to me since there but I may not have the full context of a podman secret. |
If the kube.yaml file includes a secret then the secret should be created for the run of the podman kube play command and removed when the command exits, which I believe follows the k8s standard. |
Yes, let's restore to the previous behavior. We must remain backwards compatible unless there's an urgent need not to (e.g., security fix). Thanks for pointing that out, @ashley-cui! (And sorry for not spotting that in my earlier review) |
eca895e
to
7a4a22c
Compare
@vrothberg Previous behavior restored for backward compatibility. |
Thanks, @andrei-n-cosma! The tests seem unhappy. |
7a4a22c
to
5b252ac
Compare
@vrothberg Fixed the read of k8s secret from podman secret to support both JSON and YAML. Should pass now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ashley-cui PTAL
5b252ac
to
4b86ab6
Compare
LGTM, thanks @andrei-n-cosma! |
Fixes e2e tests, remove '\n' from base64 encoded data. Correct test to check that data in secret mounted file is decoded. Closes containers#16269 Closes containers#16625 Signed-off-by: Andrei Natanael Cosma <andrei@intersect.ro>
4b86ab6
to
db4d018
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
I restarted the flaked job.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrei-n-cosma, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
will there be a v4.3.2 bugfix release? |
There are no plans to cut a v4.3.2. But 4.4 is scheduled early next year. |
Thank you for the info |
They do get backported. v4.3 had two bug-fix releases already. We didn't plan for another patch-release in this specific case as we're close to the end of the year; many folks are on vacation or shortly going to be. |
Fixes secret (un)marshaling for kube play.
Merges stringData into data for secrets as in k8s.
Fixes e2e tests, remove '\n' from base64 encoded data.
Correct test to check that data in secret mounted file is decoded.
Closes #16269
Closes #16625
Signed-off-by: Andrei Natanael Cosma andrei@intersect.ro
Does this PR introduce a user-facing change?
NO